Omit protocols for native WebSockets in browsers only. Fixes TypeErrors, see issue 82 #83
Conversation
|
Just to clarify, is this a Safari only issue? test worked fine in Chrome. |
| // special constructor treatment for native websockets in browsers, see | ||
| // https://github.com/maxogden/websocket-stream/issues/82 | ||
| if (isNative && isBrowser && !protocols) { | ||
| socket = new WS(target) |
There was a problem hiding this comment.
With this logic I can't see any way to pass protocols without also passing options as the 3rd argument. Wouldn't that cause the same error this PR is trying to fix for anyone wanting to use specific subprotocols?
There was a problem hiding this comment.
according to the w3.org, there is no options parameter for native websockets, see https://www.w3.org/TR/2011/WD-websockets-20110419/#the-websocket-interface
There was a problem hiding this comment.
Yes, but I'm talking about the optional protocols argument which is in the spec.
There was a problem hiding this comment.
right. have a look at the if clause if (isNative && isBrowser && !protocols) {
it skips when a protocol exist and executes the code in the else clause which passes on the optional protocols argument
There was a problem hiding this comment.
Wouldn't this fail in the case where you need to pass protocols to a native websockets impl? From some testing in safari it looks like any call w/ arguments.length > 2 will throw a TypeError.
ISTM we could just drop the !protocols check from the first case -- something like this:
if (isNative && isBrowser) {
socket = new WS(target, protocols)
} else {WFM at least.
There was a problem hiding this comment.
I agree that we need to pass protocols from the browsers. This is used by MQTT.js (as an example).
…rs, see issue 82.
|
Maybe a simpler solution to this would be to let ws-fallback.js drop the options argument? |
|
@mcollina no, it also happens on chrome under mac osx, not just safari |
|
@jnordberg i do not follow you - feel free to add this with or after my commit |
|
@binarykitchen I've tested with latest Chrome and latest Safari and I do not see the issue in #82. Which version of Safari/Chrome are you having issues with? |
|
@mcollina Chrome v31.0.1650.63 on Mac OS X 10.11.2 after running for two days without any restarts. Haven't tested this issue on Safari. The bug is difficult to reproduce I must admit. If you want to be 100% sure, maybe raise this on the Chromium Bug Tracker and question the native WebSocket implementation there? |
|
@binarykitchen FWIW this issue is easy for me to repro in Safari on OSX 10.10.4 (though I've never hit it in chrome). See: #83 (comment) -- this should let us pass Another alternative is just wrapping in a |
|
My problem here is that I cannot reproduce, and it does not seem an error that could pops up after 2 days of activity. @deanlandolt if you can reproduce consistently, would you mind checking if passing protocols in all cases is good, i.e. if the problem is only in passing If that's the case, I presume that is the way to go. @mafintosh @maxogden what do you think? |
|
I verified (on OSX 10.10.4 in Safari 8.0.7) that passing protocols is safe when |
|
@binarykitchen would you mind updating your PR and removing the |
|
I wasn't sure what the preferred workflow was for this case, so I just sent a PR on top of this commit's PR to @binarykitchen branch: https://github.com/binarykitchen/websocket-stream/pull/1 |
|
you can also send another PR here, I'll get that merged and released asap. |
|
sorry for my late response. was away and then github was down heh. thanks @deanlandolt for your PR. had a quick look, all looks. thanks guys! |
No description provided.